feat: (PRO-261) add signature verification flag to transaction methods by dev-jodee · Pull Request #208 · solana-foundation/kora

dev-jodee · 2025-08-29T15:54:51Z

Introduced sig_verify flag to EstimateTransactionFeeRequest, SignAndSendTransactionRequest, SignTransactionIfPaidRequest, and SignTransactionRequest for controlling signature verification during transaction simulation.
Added default_sig_verify function to provide a default value for the sig_verify flag.
Updated transaction resolution methods to utilize the sig_verify flag when simulating transactions.
Enhanced tests to cover scenarios with the new sig_verify functionality.

Important

Add sig_verify flag to transaction methods for signature verification control during simulation, with updated tests and default value function.

Behavior:
- Added sig_verify flag to EstimateTransactionFeeRequest, SignAndSendTransactionRequest, SignTransactionIfPaidRequest, and SignTransactionRequest to control signature verification during transaction simulation.
- Updated VersionedTransactionResolved::from_transaction() in versioned_transaction.rs to use sig_verify.
- Added default_sig_verify() in middleware_utils.rs to provide default value for sig_verify.
Tests:
- Enhanced tests in estimate_transaction_fee.rs, sign_and_send_transaction.rs, sign_transaction.rs, and sign_transaction_if_paid.rs to cover sig_verify scenarios.
Misc:
- Updated coverage badge logic in rust.yml to ensure correct PR description update.

^{This description was created by}^{for 6895a87. You can customize this summary. It will automatically update as commits are pushed.}

📊 Test Coverage

Coverage: 86.3%

View Detailed Coverage Report

linear · 2025-08-29T15:54:53Z

PRO-261 Simulation: allow sig verify false param

ellipsis-dev

Caution

Changes requested ❌

Reviewed everything up to 8d0b4f6 in 1 minute and 59 seconds. Click for details.

Reviewed 319 lines of code in 6 files
Skipped 0 files when reviewing.
Skipped posting 6 draft comments. View those below.
Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. crates/lib/src/rpc_server/method/sign_and_send_transaction.rs:19

Draft comment:
The 'sig_verify' flag is consistently added and passed to from_transaction. Ensure to test behavior for both true and false values.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to ensure that the behavior is tested for both true and false values of the 'sig_verify' flag. This falls under the rule of not asking the author to ensure the behavior is tested, which is not allowed.

2. crates/lib/src/rpc_server/method/sign_transaction_if_paid.rs:19

Draft comment:
The new 'sig_verify' flag is applied uniformly in this endpoint. Testing with both true and false could further ensure robustness.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 50% <= threshold 50% The comment suggests testing with both true and false values for a new flag, which is a specific suggestion for testing. However, it also includes a general statement about ensuring robustness, which is not allowed. The comment is partially useful but needs to be more specific.

3. crates/lib/src/rpc_server/middleware_utils.rs:5

Draft comment:
The helper function 'default_sig_verify' correctly returns true. A brief doc comment here could further clarify its purpose for future readers.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is purely informative and suggests adding documentation for clarity. It doesn't provide a specific code suggestion or point out a potential issue with the code.

4. crates/lib/src/transaction/versioned_transaction.rs:146

Draft comment:
Passing 'sig_verify' into RpcSimulateTransactionConfig is well done. Consider handling bs58 decode errors explicitly (instead of unwrap_or_default) in inner instructions parsing to avoid silently masking issues.
Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.

5. crates/lib/src/rpc_server/method/sign_and_send_transaction.rs:16

Draft comment:
Typo: In the comment on line 16, "Optional signer signer_key..." repeats the word "signer". Consider revising to something like "Optional signer_key to ensure consistency..."
Reason this comment was not posted:
Comment was on unchanged code.

6. crates/lib/src/rpc_server/method/sign_transaction_if_paid.rs:15

Draft comment:
Typographical error: The comment for signer_key redundantly says "Optional signer signer_key". Consider removing the extra 'signer' to read "Optional signer_key to ensure consistency across related RPC calls".
Reason this comment was not posted:
Comment was on unchanged code.

Workflow ID: wflow_oVA7mdSa0NT3xUBn

^{You can customize}^{by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

crates/lib/src/rpc_server/method/estimate_transaction_fee.rs

crates/lib/src/rpc_server/method/sign_transaction.rs

- Introduced `sig_verify` flag to `EstimateTransactionFeeRequest`, `SignAndSendTransactionRequest`, `SignTransactionIfPaidRequest`, and `SignTransactionRequest` for controlling signature verification during transaction simulation. - Added `default_sig_verify` function to provide a default value for the `sig_verify` flag. - Updated transaction resolution methods to utilize the `sig_verify` flag when simulating transactions. - Enhanced tests to cover scenarios with the new `sig_verify` functionality.

ellipsis-dev

Important

Looks good to me! 👍

Reviewed 3fbd386 in 40 seconds. Click for details.

Reviewed 63 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 2 draft comments. View those below.
Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. crates/lib/src/transaction/versioned_transaction.rs:889

Draft comment:
New async test 'test_fetch_inner_instructions_with_sig_verify_false' correctly tests the behavior when the simulation is run with sig_verify set to false. The assertions and mock setup look good. Consider adding a complementary test that simulates an error scenario to ensure robust error handling when signature verification is disabled.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50% None

2. crates/lib/src/transaction/versioned_transaction.rs:889

Draft comment:
The new test is very similar to the existing one for sig_verify=true. Consider refactoring or parameterizing tests to reduce duplication and improve maintainability.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_Ib4GYXBItaATI8uG

^{You can customize}^{by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

ellipsis-dev

Important

Looks good to me! 👍

Reviewed 17c6b6f in 56 seconds. Click for details.

Reviewed 63 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 2 draft comments. View those below.
Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. crates/lib/src/transaction/versioned_transaction.rs:894

Draft comment:
New test 'test_fetch_inner_instructions_with_sig_verify_false' correctly validates that inner instructions are fetched when 'sig_verify' is false. Good coverage for the new flag.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50% None

2. crates/lib/src/transaction/versioned_transaction.rs:895

Draft comment:
Consider adding a brief inline doc comment in this test (or in the related function) to explain the significance of the 'sig_verify' flag, clarifying when and why to disable signature verification during simulation.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_nLhDumOCZVdI32wP

^{You can customize}^{by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

ellipsis-dev

Important

Looks good to me! 👍

Reviewed 194d4a9 in 43 seconds. Click for details.

Reviewed 25 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 2 draft comments. View those below.
Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. tests/src/common/assertions.rs:83

Draft comment:
Update comment to reflect that valid blockhash lengths range between 43 and 44 characters. Ensure that tests also cover both scenarios.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50% None

2. tests/src/common/assertions.rs:99

Draft comment:
Replaced assert_eq! with a range check for blockhash length. Consider verifying that relying on Rust's implicit format capture (using '{blockhash}') is acceptable for your minimum Rust version.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_TNmrsdwqqAcVrLq7

^{You can customize}^{by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

ellipsis-dev

Important

Looks good to me! 👍

Reviewed b008487 in 34 seconds. Click for details.

Reviewed 13 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 1 draft comments. View those below.
Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. .github/workflows/rust.yml:174

Draft comment:
Good change: the added condition (github.event.pull_request.head.repo.full_name == github.repository) ensures that the coverage badge update only runs for PRs created from the same repository, preventing potential permission issues with forks. Consider adding an inline comment explaining this rationale for future maintainers.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is purely informative, explaining the rationale behind a code change. It does not ask for any specific action or suggest any improvements. According to the rules, purely informative comments should be removed.

Workflow ID: wflow_LVMesNSOd4XUA6Xs

^{You can customize}^{by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

ellipsis-dev

Important

Looks good to me! 👍

Reviewed 5ffc4c6 in 1 minute and 6 seconds. Click for details.

Reviewed 13 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 1 draft comments. View those below.
Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. crates/lib/src/rpc_server/middleware_utils.rs:6

Draft comment:
Default value for sig_verify changed from true to false. Confirm if disabling signature verification by default is intended and update docs if necessary.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment violates several rules: 1) It asks for confirmation of intention ("Confirm if...") 2) It asks for documentation updates which we shouldn't do 3) It's not clearly pointing out a bug or suggesting a concrete improvement. The change itself is simple and deliberate - someone explicitly changed this boolean from true to false. This could be a security-critical change that deserves attention. Disabling signature verification by default could introduce vulnerabilities. While security is important, the comment doesn't actually point out a concrete issue - it just asks for confirmation. If there was a security concern, it should be stated directly. Delete this comment. It violates our rules by asking for confirmation and documentation updates without pointing out any concrete issues.

Workflow ID: wflow_Rk5Ie6qo24zXwVZU

^{You can customize}^{by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

ellipsis-dev

Important

Looks good to me! 👍

Reviewed aeb8e70 in 43 seconds. Click for details.

Reviewed 17 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 1 draft comments. View those below.
Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. crates/lib/src/transaction/versioned_transaction.rs:148

Draft comment:
Explicitly setting the commitment (using rpc_client.commitment()) in the simulation config is a good improvement for consistency. Consider adding a brief comment to explain this decision for future maintainers.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_nodaCpNrxKP9oTpa

^{You can customize}^{by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

ellipsis-dev

Important

Looks good to me! 👍

Reviewed 6895a87 in 36 seconds. Click for details.

Reviewed 7 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 1 draft comments. View those below.
Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. .github/badges/coverage.json:1

Draft comment:
Ensure that the manual update to the coverage badge (86.2% from 86.3%) is intentional. Consider automating badge updates to avoid manual adjustments in the future.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_JrplzXEBeCTl2Vok

^{You can customize}^{by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

dev-jodee requested a review from amilz August 29, 2025 15:54